Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[experimental] update test-demo with version with lambdas #1146

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

barendgehrels
Copy link
Collaborator

Hi, this is an experimental version of partition using lambdas.

For demonstration and evaluation purposes.

I'm curious to your findings.

@barendgehrels barendgehrels self-assigned this May 8, 2023
return true;
},
box_visitor
);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shows how the function could be used.
5 lambdas (2 expand, 2 overlaps, 1 main) plus an optional lambda (called per box),
instead of the earlier policies with apply

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The program itself creates SVG's like this:

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot better! I prefer function objects over objects with apply() function.

One nitpick. This is not quadtree, it's kdtree, a node in quadtree is divided into 4 children, here a division is done into 2, once per dimension. Furthermore partition is k-dimensional, quadtree is 2-dimensional.

Side note, I was thinking about redesigning the API for a long time and use features from the rtree. Note e.g. that in your example you shouldn't be forced to pass function objects defining how to expand a box or perform intersection. These operations are always done the same way. We probably also never use box visitors, besides debugging or visualization so box visitor could be hidden as well. The only additional information in your example that's needed is the way of getting a box from item (taking point from point should work by default). In rtree this is done by IndexableGetter. There are also additional things like the type of operation (in this case intersects) and strategies (for defining CS) but they could be optional as well. I'm not saying to do this, only sharing what I was thinking about.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

I think it's not exactly a kdtree either. You are right, we divide into halves and not into 4. But always at the middle, in kdtree that it is not necessarily the case. Anyway, I can rename it of course. But it cannot be called partition... (unless we replace it).
Plus we have those additional boxes for cases where geometries exceed the halves. But sure, they are not part of the quadtree either.

I see your point. Yes, taking the box from an object is usually enough. Or point. But it could be done in a different way as well. For example: a box with a margin (for (max) distance calculations). It's flexible now. But that flexibility is probably used rarely, if at all. We use partition only internally, I think. It was introduced before our index so at that moment it was not a choice. So yes, it can be a bit simpler. The box visitor is indeed only used for debugging (at least at this moment).

Anyway, additionally, if we go this way, we don't need those visitor-adaptor(s), we can deprecate the old implementation and let the new one immediately use function objects.

Copy link
Member

@awulkiew awulkiew May 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Splitting planes can be placed at various locations in a kd-tree. Classic approach is to place them at the median object. Another strategy is to cut empty space out. There is also version using surface area heuristic. There are probably more. It depends on a use case. Splitting in half is probably a poor choice in general good only for objects distributed uniformly but such tree is still kd-tree.

Btw, in kd-tree splitting doesn't have to be done one dimension after another. The space can be split e.g. in X axis cutting out empty space and then again in X axis at the middle object, etc. But no matter what strategy is used this is still kd-tree.

Regarding quadtrees. There are versions of quadtrees that have splitting planes at different locations than geometric middle of a node. AFAIR there are also quadtrees with child nodes partially overlapping each other which makes them similar to rtrees.

Obviously partition is neither quadtree nor kd-tree because it's not a data structure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're completely right, it's not a data structure. Thanks for the backgrounds. I will just keep the partition name.
As a work-title it's now temporarily lambda_partition.
I will not merge this PR. It is a discussion piece. If we agree and things are clear, I'll change the original (copy, and deprecate the old one)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's used internally around 12 times.
Unfortunately it's not in namespace detail so officially it is part of the interface.

>
bool quadtree_partition(ForwardRange1 const& forward_range1,
ForwardRange2 const& forward_range2,
const std::function
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd avoid using std::function because it's heavy. Why did you decide to use it over a function object?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The compiler error messages are clearer.
I was not aware that it is heavier.

Comment on lines 23 to 37
template <typename F>
struct adapt_partition_visitor
{
const F& m_f;

explicit adapt_partition_visitor(const F& f)
: m_f(f)
{}

template<typename T1, typename T2>
bool apply(T1 const& i1, T2 const& i2) const
{
return m_f(i1, i2);
}
};
Copy link
Member

@awulkiew awulkiew May 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of the adaptors could probably be replaced with one simply forwarding parameters to function object:

template <typename F>
struct adapt_partition_visitor
{
    F m_f;

    explicit adapt_partition_visitor(F&& f)
        : m_f(std::move(f))
    {}

    template <typename ...Ts>
    decltype(auto) apply(Ts&& ...is) const
    {
        return m_f(std::forward<Ts>(is)...);
    }
};

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, works perfectly. Thanks!
✔️

@barendgehrels barendgehrels force-pushed the experimental-quadtree-partition branch 2 times, most recently from 33e1218 to dc9c4cc Compare May 10, 2023 09:07
Copy link
Member

@vissarion vissarion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is cool! I am OK with merging now.
I will have a deeper look next week.
Regarding the name I also agree it is neither quad nor kd-tree (it is not a tree at all of course and it actually combines the default splitting strategy "cut in middle" of quad trees with the kd-tree per-dimension partitioning). So I think it is better to just call it "partitioning".

@@ -0,0 +1,17 @@
# Boost.Geometry (aka GGL, Generic Geometry Library)
# Robustness Test - convex_hull
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not convex hull, typo?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, I copied another file and forgot to adapt the title.
✔️ fixed

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is cool! I am OK with merging now. I will have a deeper look next week. Regarding the name I also agree it is neither quad nor kd-tree (it is not a tree at all of course and it actually combines the default splitting strategy "cut in middle" of quad trees with the kd-tree per-dimension partitioning). So I think it is better to just call it "partitioning".

Renamed now to partitioning (currently in namespace experimental)

detail::partition::include_all_policy
>::apply(forward_range1, forward_range2, av,
aev1, aov1,
aev2, aov2, 16, apbv);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe add a comment that 16 is the end of the iteration (the max number of points in a partition box)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it to an options structure, as I planned earlier.
Is something like this fine?
✔️

@barendgehrels barendgehrels force-pushed the experimental-quadtree-partition branch 2 times, most recently from 17ae254 to 7251ca5 Compare May 12, 2023 17:33
@barendgehrels barendgehrels added the experimental Experimental PR - might need work or discussion label Jul 2, 2023
@barendgehrels barendgehrels force-pushed the experimental-quadtree-partition branch from 7251ca5 to 0b4bfe7 Compare September 29, 2023 12:47
@barendgehrels
Copy link
Collaborator Author

I updated this PR:

  • adding a lambda version over one range as well (like the original had)
  • applying the lambda version in buffer, which nicely removes many helper structures.

It's still in namespace experimental (so don't merge yet), I can't put it in namespace geometry because it conflicts with the structure partition. Any suggestions?

{
return turn.is_turn_traversable && ! turn.within_original;
}
};
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing this, I noticed that we sometimes use the include_turn_policy but sometimes have it in the get_box/overlaps functors. For that reason, if this functionality is always there, we could also choose to get rid of these options.
(the one left is the default partition size but we can specify it as an optional parameter).
What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I omitted it, it was really just about expanding the bounding box.

template
<
typename BoxType,
typename ForwardRange,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This version is added, it uses only one ForwardRange which is partitioned with itself

@barendgehrels barendgehrels force-pushed the experimental-quadtree-partition branch from 0b4bfe7 to d789038 Compare September 29, 2023 12:57
@barendgehrels
Copy link
Collaborator Author

I updated this PR:

  • adding a lambda version over one range as well (like the original had)
  • applying the lambda version in buffer, which nicely removes many helper structures.

It's still in namespace experimental (so don't merge yet), I can't put it in namespace geometry because it conflicts with the structure partition. Any suggestions?

I'm now using detail::lambda_partition.
Still to do:

  • use it in disjoint
  • use it in is_valid
  • use it in relate
    So it's getting there.

Then we can deprecate partition itself because it is not used inside the library.

In a next Boost version, we can delete it and rename partition_lambda to just partition.

@barendgehrels
Copy link
Collaborator Author

gcc 5 now reports:

/home/gehrels/git_public/boost/boost/geometry/algorithms/detail/overlay/get_turns.hpp:524:32: error: request for member ‘bounding_box’ in ‘sec’, which is of non-class type ‘const int’
                geometry::expand(box, sec.bounding_box, strategy);

This is inconvenient and wrong, it is there and correct. And it is only for the second lambda...

Any idea how to circumvent this?

@barendgehrels
Copy link
Collaborator Author

which is of non-class type ‘const int’

It confuses the two overloads of partition_lambda... If I use different names, it works. But that is less convenient....
To be continued.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experimental Experimental PR - might need work or discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants